[fix](ci) rm some infos#63228
Merged
Merged
Conversation
Contributor
Author
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
Contributor
Author
|
run buildall |
Contributor
Author
TPC-H: Total hot run time: 29701 ms |
Contributor
Author
TPC-DS: Total hot run time: 169982 ms |
Contributor
Author
|
/review |
zhangstar333
previously approved these changes
Jun 2, 2026
github-actions
Bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
I found a blocking issue in the credential redaction change. The PR goal is clear and focused, but the current implementation does not reliably accomplish it because optional earlier credentials can make the whole sed script invalid before required later credentials are processed.
Critical checkpoint conclusions:
- Goal/test: goal appears to be redacting additional AK/SK values from archived logs/configs; no test evidence was provided, and the shell behavior shows a normal missing-optional-variable case is not covered.
- Scope: change is small and focused.
- Concurrency/lifecycle: not applicable; this is a CI archive helper.
- Configuration/dynamic config: no new config items.
- Compatibility/storage/transactions/data writes: no Doris storage or transaction format impact.
- Parallel paths: the same unsafe sed pattern is duplicated for FE logs, BE logs, regression logs, and regression conf; fix all occurrences consistently.
- Conditional checks/error handling: sed failure is intentionally ignored, which is acceptable only if redaction is best-effort, but here it silently leaves required secrets in the archive.
- Test coverage/results: missing coverage for partially unset credentials.
- Observability/performance: no material concerns.
User focus: no additional user-provided review focus was specified.
| ( | ||
| cd "${DORIS_HOME}" || return 1 | ||
| cp --parents -rf "fe/conf" "${archive_dir}"/ | ||
| if sed -i "s/${hwYunAk:-}//g;s/${hwYunSk:-}//g;s/${s3SourceAk:-}//g;s/${s3SourceSk:-}//g;s/${txYunAk:-}//g;s/${txYunSk:-}//g" fe/log/* &>/dev/null; then :; fi |
Contributor
There was a problem hiding this comment.
This redaction can be skipped in normal cloud/vault runs when the optional Huawei credentials are unset. cloud_p0/run.sh and vault_p0/run.sh require s3SourceAk/s3SourceSk but only warn for missing hwYunAk/hwYunSk; with hwYunAk empty this expands to an invalid leading sed expression (s//g...), sed exits before processing the later required S3 credentials, and the failure is suppressed by &>/dev/null, so the archive can still contain s3SourceAk/s3SourceSk. Please build the redaction script only from non-empty values (and apply the same fix to the duplicated BE/regression/conf redaction lines) instead of interpolating empty variables directly.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions
Bot
removed
the
approved
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Summary opinion: request changes. The PR is focused and addresses the already-raised empty-variable sed issue, but the replacement helper still has a blocking credential-leak path because secret values are used as raw sed patterns/delimiters and sed failures are suppressed.
Critical checkpoint conclusions:
- Goal/test: The goal is to redact cloud credentials from archived Doris logs/configs. Current code does not fully accomplish it for valid credentials containing sed metacharacters; no test is included to prove redaction across such values.
- Scope: The change is small and focused.
- Concurrency/lifecycle: No new concurrency or special lifecycle management.
- Configuration/compatibility: No new configuration, storage format, or compatibility concern.
- Parallel paths: The helper is applied to FE logs, BE logs, regression logs, and regression conf, which covers the modified archive paths.
- Conditional checks: Empty credential values are now skipped, which addresses the known prior review thread.
- Test coverage/results: No automated test coverage was added for the shell redaction behavior; this is important because failures are intentionally silenced.
- Observability: No additional observability needed, but silent sed failure makes the archive unsafe unless the redaction implementation is robust.
- Transactions/data writes: Not applicable.
- FE/BE variable passing: Not applicable.
- Performance: Negligible impact.
- Other issues: Raw sed interpolation can fail or mis-redact secrets containing '/', '[', '\', or other regex syntax, leaving credentials in uploaded archives.
User focus: No additional user-provided review focus was specified.
| _redact_creds() { | ||
| local expr="" v | ||
| for v in "${hwYunAk:-}" "${hwYunSk:-}" "${s3SourceAk:-}" "${s3SourceSk:-}" "${txYunAk:-}" "${txYunSk:-}"; do | ||
| [[ -n "${v}" ]] && expr+="s/${v}//g;" |
Contributor
There was a problem hiding this comment.
This still leaves a credential leak path for valid secret values that contain sed syntax. For example, an S3 secret key can contain /; with s3SourceSk='abc/def' this builds s/abc/def//g;, sed fails, and the helper suppresses the failure with || true, so the logs/configs are copied into the archive unredacted. Regex metacharacters such as [ can also make the pattern invalid or overmatch. Please escape the sed pattern/delimiter for each value, or use a fixed-string/quoted redaction approach, before suppressing redaction command failures.
…tion Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review summary for PR 63228 at f537326.
Overall opinion: no new blocking issue found in the current diff. The prior review threads about empty credential values and sed metacharacter handling were considered as already-known context; the current helper now skips empty values and escapes the expected sed regex and delimiter metacharacters before redaction.
Critical checkpoint conclusions:
- Goal and proof: The PR focuses on preventing credential leakage when archiving regression logs/configs. The implementation is consistent with that goal for the modified call sites; I verified the script parses with
bash -nand checked the changed script withshellcheck. - Scope and clarity: The change is small and localized to
regression-test/pipeline/common/doris-utils.sh. - Concurrency: No new concurrency or shared-state lifecycle concerns; this helper runs during archive collection.
- Lifecycle/static initialization: Not applicable for this shell-script change.
- Configuration items: No new config item added.
- Compatibility/storage formats/protocols: Not applicable.
- Parallel code paths: The same helper is used for the FE log, BE log, regression log, and regression conf archive paths touched by the PR.
- Conditional checks: The non-empty credential filter avoids the previous empty sed expression failure path.
- Tests/results: No regression test output is involved. Local checks run:
bash -n regression-test/pipeline/common/doris-utils.sh;shellcheck regression-test/pipeline/common/doris-utils.shreported one informational SC2015 note on the changedA && B || truepattern and pre-existing warnings near lines 1026-1032, but no syntax failure. - Observability: Not applicable beyond archive redaction behavior.
- Transactions/persistence/data writes: Not applicable.
- FE/BE variable passing: Not applicable.
- Performance: The added per-secret escaping and in-place sed calls are small and only on archive collection paths.
User focus points: No additional user-provided review focus was supplied, and no extra issue was found related to focus points.
zhangstar333
approved these changes
Jun 8, 2026
Contributor
Author
|
skip buildall |
github-actions Bot
pushed a commit
that referenced
this pull request
Jun 8, 2026
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [ ] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg:
apache/doris-website#1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR
should merge into -->
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
github-actions Bot
pushed a commit
that referenced
this pull request
Jun 8, 2026
### What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
### Release note
None
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [ ] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [ ] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [ ] No.
- [ ] Yes. <!-- Add document PR link here. eg:
apache/doris-website#1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR
should merge into -->
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #xxx
Related PR: #xxx
Problem Summary:
Release note
None
Check List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)